feat: add --ignore-env=...
#31187
feat: add --ignore-env=...
#31187
Conversation
|
Closes #31071? |
|
Can you please update the docs for permissions and show an example what happens if you use |
| fn check_env_with_maybe_exit( | ||
| state: &mut OpState, | ||
| key: &str, | ||
| ) -> Result<ControlFlow<()>, PermissionCheckError> { |
|
Not a blocker, but we should update the interactive prompt with the |
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for ignoring specific environment variables through the new --ignore-env flag and ignore configuration option. When an environment variable is ignored, attempts to access it return undefined (or are excluded from Deno.env.toObject()) without raising a permission error, providing a way to hide sensitive environment variables from scripts without requiring explicit permission denials.
Key Changes:
- Introduces new
PermissionState::IgnoredandPermissionState::DeniedPartialstates to the permission system - Adds
--ignore-envCLI flag and corresponding configuration support for environment permissions - Refactors
NODE_ENV_VAR_ALLOWLISTfrom a lazy-initializedHashSetto a sorted const array for more efficient lookups
Reviewed Changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
tests/specs/run/permission_env_allow_and_deny/main.ts |
Updates test to verify env vars are properly filtered with allow/deny flags |
tests/specs/run/permission_env_allow_and_deny/main.out |
Removes expected error output as test now succeeds with proper permission setup |
tests/specs/run/permission_env_allow_and_deny/__test__.jsonc |
Updates test config to use allow/deny flags without expecting errors |
tests/specs/permission/ignore_env/flags/main.ts |
New test for --ignore-env flag functionality |
tests/specs/permission/ignore_env/flags/__test__.jsonc |
Test configurations for various --ignore-env scenarios (all, some, with allow-env, with deny-env) |
tests/specs/permission/ignore_env/config/main.ts |
New test for ignore configuration via deno.json |
tests/specs/permission/ignore_env/config/deno.json |
Config file demonstrating ignore: true setting for env permissions |
tests/specs/permission/ignore_env/config/__test__.jsonc |
Test configuration for ignore via config file |
runtime/permissions/lib.rs |
Core implementation: adds Ignored/DeniedPartial states, implements ignore list tracking, updates permission query logic |
runtime/ops/permissions.rs |
Updates PermissionStatus conversion to handle new permission states |
libs/config/deno_json/permissions.rs |
Adds AllowDenyIgnorePermissionConfig struct and deserialization support |
libs/config/deno_json/mod.rs |
Exports new AllowDenyIgnore config types |
ext/os/lib.rs |
Implements ignore behavior in env operations (get/set/delete/toObject), refactors NODE_ENV_VAR_ALLOWLIST to const array |
cli/schemas/config-file.v1.json |
Defines JSON schema for ignore permission config |
cli/args/mod.rs |
Integrates ignore_env into permissions options, renames no_op to identity for clarity |
cli/args/flags.rs |
Adds ignore_env flag parsing, serialization, and test coverage |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <[email protected]> Signed-off-by: David Sherret <[email protected]>
| "deny": { "$ref": "#/$defs/permissionConfigValue" }, | ||
| "ignore": { "$ref": "#/$defs/permissionConfigValue" } | ||
| } | ||
| }, |
There was a problem hiding this comment.
This needs to be used in "env" before merging.
dsherret
force-pushed
the
feat_allow_env_deny_to_none
branch
from
8185f89 to
f658c29
Compare
|
Feels like it closes #29471 too |
Not sure about that because we might want to expand the number of allowed env variables by default. |
bartlomieju
left a comment
bartlomieju
left a comment
There was a problem hiding this comment.
Couple comments, let's get some more eyes on this one too.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds an "ignore" permission path for environment variables: schema/types and config parsing to accept an ignore list, CLI flag plumbing for Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as CLI / Flags
participant Config as Config Parser
participant PermOpts as PermissionsOptions
participant PermLib as Permission Lib
participant EnvOp as Env Operation
User->>CLI: deno run --ignore-env=VAR app.ts
CLI->>Config: parse flags + config
Config->>PermOpts: set ignore_env: ["VAR"]
PermOpts->>PermLib: construct permissions (new_unary_with_ignore)
PermLib->>PermLib: create FlagIgnored descriptor for "VAR"
User->>EnvOp: Deno.env.get("VAR")
EnvOp->>PermLib: check_env("VAR")
PermLib-->>EnvOp: PermissionState::Ignored
EnvOp->>User: return undefined
User->>EnvOp: Deno.env.toObject()
EnvOp->>PermLib: check env for each key
PermLib-->>EnvOp: Ignored for "VAR", Granted for others
EnvOp->>User: return object omitting Ignored keys
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
cli/args/flags.rs (2)
4204-4249: Help text omission: add--ignore-envto Permission options.The after_help block documents allow/deny env, but not ignore. Add it so users can discover this flag.
Apply this diff:
@@ <g>--deny-env[=<<VARIABLE_NAME>...]</> Deny access to environment variables. Optionally specify inacessible environment variables. <p(245)>--deny-env | --deny-env="PORT,HOME,PATH"</> + <g>--ignore-env[=<<VARIABLE_NAME>...]</> Ignore reads of environment variables. Optionally specify variable names; when none are provided, all env reads return 'undefined' and env enumeration is empty. + <p(245)>--ignore-env | --ignore-env="PORT,HOME,PATH"</>
1261-1279:has_permission_in_argv()misses--ignore-env.This causes false negatives when only
--ignore-envis supplied. Include it (and consider--allow/--deny-import, but that’s optional here).Apply this diff:
@@ pub fn has_permission_in_argv(&self) -> bool { self.argv.iter().any(|arg| { arg == "--allow-all" || arg.starts_with("--allow-env") || arg.starts_with("--deny-env") + || arg.starts_with("--ignore-env") || arg.starts_with("--allow-ffi") || arg.starts_with("--deny-ffi") || arg.starts_with("--allow-net") || arg.starts_with("--deny-net") || arg.starts_with("--allow-read") || arg.starts_with("--deny-read") || arg.starts_with("--allow-run") || arg.starts_with("--deny-run") || arg.starts_with("--allow-sys") || arg.starts_with("--deny-sys") || arg.starts_with("--allow-write") || arg.starts_with("--deny-write") }) }Also extend the test to cover this case:
@@ fn has_permission_in_argv() { - let r = flags_from_vec(svec!["deno", "run", "x.ts"]); + let r = flags_from_vec(svec!["deno", "run", "x.ts"]); assert_eq!(r.unwrap().has_permission_in_argv(), false); + + let r = flags_from_vec(svec!["deno", "run", "x.ts", "--ignore-env"]); + assert_eq!(r.unwrap().has_permission_in_argv(), true);
♻️ Duplicate comments (2)
cli/args/mod.rs (1)
1511-1715:ignore_envmapping reuseshandle_denysemantics appropriatelyWiring
ignore_envthroughhandle_denygives it the same precedence rules as the other list-style permissions (flags override config;All→ empty vec sentinel;Some→ normalized list), which matches how ignore lists are conceptually used here. The only minor nit is thathandle_denyis now a generic “list from flags/config” helper rather than strictly deny-specific; if you ever touch this again, a more neutral name (or a brief comment noting it’s shared with ignore) could make the intent clearer.runtime/permissions/lib.rs (1)
721-811: Ordering and precedence for FlagDenied/PromptDenied/FlagIgnored are correct but precedence u8 remains opaqueThe refactor of
UnaryPermissionDescordering to carry an explicit precedence for the deny variants and the addition ofFlagIgnoredintegrates cleanly:
AllowOrDenyDescRef::Deny(_, precedence)pluskind_precedence()ensure a stable sort whereFlagDenied < PromptDenied < FlagIgnored < Grantedfor equal descriptors.UnaryPermissionDescriptorstrackshas_flag_ignoredand the combinedhas_any_denied_or_ignored()so that higher‑level logic can cheaply reason about any non‑granted, non‑prompt state.Functionally this is solid and well covered by the comparison tests at the end of the file. The only readability nit is that raw
u8precedence values (0/1/2) are not self‑describing; an internal enum for the precedence level would make this easier to maintain, which matches the earlier reviewer suggestion.Also applies to: 813-889
🧹 Nitpick comments (10)
cli/args/flags.rs (2)
9228-9247: Consider adding validator tests for--ignore-env.Parity with allow/deny validators would help catch invalid keys (
=,\0) for ignore too.You can add:
@@ fn ignore_env_ignorelist() { @@ } + + #[test] + fn ignore_env_ignorelist_validator() { + let r = flags_from_vec(svec!["deno", "run", "--ignore-env=HOME", "script.ts"]); + assert!(r.is_ok()); + let r = flags_from_vec(svec!["deno", "run", "--ignore-env=H=ME", "script.ts"]); + assert!(r.is_err()); + let r = flags_from_vec(svec!["deno", "run", "--ignore-env=H\0ME", "script.ts"]); + assert!(r.is_err()); + }
9292-9315: Test name typo.The test uses
--ignore-envbut is nameddeny_env_ignorelist_multiple. Rename for clarity.Apply this diff:
- fn deny_env_ignorelist_multiple() { + fn ignore_env_ignorelist_multiple() {libs/config/deno_json/permissions.rs (2)
128-165: Consider de-duplicatingdeserialize_allow_deny_ignoreanddeserialize_allow_denyThe bool and list branches here are effectively identical to
deserialize_allow_deny, only differing in the target struct type and absence/presence ofignore. A small shared helper (parameterized over the target struct) would reduce the chance of these diverging on future changes.
308-312: Updated serde tests forenvstay aligned with new typeThe expectations for
envnow usingAllowDenyIgnorePermissionConfig { allow: Some(...), deny: None, ignore: None }keep the tests validating backwards-compatible behavior forenv: trueandenv: ["test"]. You might optionally add a case that exercisesenv: { "ignore": true }(or an ignore list) at this level to pin the new semantics in unit tests, but the spec tests will already cover behavior end-to-end.Also applies to: 358-362
tests/specs/permission/ignore_env/flags/main.ts (1)
1-7: Flags test script accurately asserts ignore-env behaviorThe script correctly verifies that
VAR1reads and enumeration are suppressed when ignored while still allowingVAR2. For slightly better diagnostics you couldthrow new Error("FAILED")instead of a bare string, but that’s strictly optional in this test context.tests/specs/permission/ignore_env/config/main.ts (1)
1-7: Test script correctly validates ignore-env behaviorLogging
Deno.env.get("VAR1"),Deno.env.get("VAR2"), and asserting"VAR1" in objectfails covers the core semantics of ignored vars disappearing fromtoObject(). If you want slightly clearer stack traces, you could throw anErrorinstead of a string:-if ("VAR1" in object) { - throw "FAILED"; -} +if ("VAR1" in object) { + throw new Error("FAILED"); +}tests/specs/permission/ignore_env/flags/__test__.jsonc (1)
1-26: Flag tests give good coverage of ignore/allow/deny env combinationsThe four cases (
--ignore-env,--ignore-env=VAR1,--ignore-env=VAR1 --allow-env, and--deny-env --ignore-env=VAR1) nicely exercise the intended interactions and should catch regressions around ignored vs denied variables and enumeration behavior.If you want the fixtures to be a bit more uniform, you could align the error output formatting (spacing after
error:and trailing newlines) acrosssomeanddeny_envto avoid subtle snapshot differences, but that’s purely cosmetic.ext/os/lib.rs (2)
7-7: Env permission helper and Ignored handling look consistent; confirm silent no-op semantics for set/deleteThe new
check_env_with_maybe_exithelper cleanly centralizes env checks and the mapping ofPermissionState::IgnoredtoControlFlow::Break(()), and its integration intoop_get_env,op_set_env, andop_delete_envis coherent: denied states still surface asOsError::Permission, while ignored states short‑circuit the ops.One behavioral nuance worth double‑checking: for ignored env vars,
op_set_envandop_delete_envnow become silent no‑ops (returningOk(())without touching the process env), andop_get_envreturnsOk(None). That means JS callers will seeDeno.env.set/deletesucceed andDeno.env.get/toObject()behave as if the variable doesn’t exist. If the intent was to treat ignore as “read returns undefined” only, rather than also suppressing writes/deletes, you may want to reconsider whether writes should still error out for ignored vars.Also applies to: 151-177, 179-192, 269-281, 283-300
206-218: Env enumeration behavior matches ignore semantics; consider aligning/proc/self/environhandlingThe
op_envchanges correctly makecheck_env_all()treatGrantedPartial,DeniedPartial, andIgnoredas “no global grant” while still proceeding with enumeration and filtering Ignored/Denied entries out of the returned map, which aligns with the--ignore-env/--allow-env+--deny-envgoals.Note that
PermissionsContainer::check_env_all()is still used directly incheck_special_filefor/proc/self/fd/.../environ, wherePermissionState::Ignoredand partial denials will still bubble up as hard errors. If you want/proc/self/environreads to behave consistently withDeno.env.toObject()under ignore/deny configurations, it may be worth special‑casing Ignored/partial states there as well.Also applies to: 223-236, 4205-4227
runtime/permissions/lib.rs (1)
6507-6563: test_env_ignore covers key patterns; consider adding a global-ignore caseThe new
test_env_ignorenicely validates:
- Simple allow/deny/ignore patterns (
ALLOWED_*,DENIED_*,IGNORED_*), and- Overlapping patterns where ignore and allow both refine a broader deny (
PREFIX_ALLOWED*,PREFIX*,PREFIX_IGNORED*),with the expected
Granted/Denied/Ignoredoutcomes.One additional edge case you might want to cover here (even if CLI specs already test it) is the “global ignore” configuration where
ignore_envis an explicit empty list (e.g.,Permissions::new_unary_with_ignore(None, None, Some(Vec::new()), false)) to assert that all env queries returnPermissionState::Ignoredand thatcheck_all()behaves as expected in that mode.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
cli/args/flags.rs(8 hunks)cli/args/mod.rs(5 hunks)cli/schemas/config-file.v1.json(2 hunks)ext/os/lib.rs(7 hunks)libs/config/deno_json/mod.rs(1 hunks)libs/config/deno_json/permissions.rs(5 hunks)runtime/ops/permissions.rs(1 hunks)runtime/permissions/lib.rs(25 hunks)tests/specs/permission/ignore_env/config/__test__.jsonc(1 hunks)tests/specs/permission/ignore_env/config/deno.json(1 hunks)tests/specs/permission/ignore_env/config/main.ts(1 hunks)tests/specs/permission/ignore_env/flags/__test__.jsonc(1 hunks)tests/specs/permission/ignore_env/flags/main.ts(1 hunks)
🔇 Additional comments (18)
cli/args/flags.rs (6)
817-817: New permission flag field looks good.Serde + Default semantics are correct for a new optional field; no compatibility concerns.
840-841: Correctly included in has_permission().Good to account for ignore_env here.
979-988: to_permission_args: correct emission of--ignore-env.Handles both empty (all ignored) and specific values. LGTM.
4179-4202: Helper for env-based deny/ignore flags is sound.Consistent validation and Windows uppercasing match existing allow-env behavior.
4407-4408: Argument wiring:--ignore-envcorrectly added.Reuses the shared validator; matches
--deny-env. LGTM.
6639-6642: Parser plumbing forignore-envis correct.Values flow into
flags.permissions.ignore_envand are logged for debug.libs/config/deno_json/permissions.rs (2)
80-92: NewAllowDenyIgnorePermissionConfigstruct is consistent with existing patternThe shape and
is_nonesemantics line up withAllowDenyPermissionConfigwhile correctly treating any non-Noneignoreas “configured.” This keepsPermissionsObject::is_emptybehavior coherent when env ignore is used.
191-192:envwiring toAllowDenyIgnorePermissionConfiglooks correctSwitching
envtoAllowDenyIgnorePermissionConfigwithdeserialize_allow_deny_ignorepreserves existing bool/array semantics while enablingenv: { "ignore": ... }in configs.PermissionsObject::is_emptywill continue to work via the newis_noneimplementation.tests/specs/permission/ignore_env/config/__test__.jsonc (1)
1-8: Config-based ignore-env test spec matches intended semanticsThe env setup, args (
run --quiet -P main.ts), and expected output (undefinedreads plus an empty[Object: null prototype] {}) correctly validate that defaultenv.ignore: truecauses all env probes and enumeration to be ignored.tests/specs/permission/ignore_env/config/deno.json (1)
1-9:permissions.default.env.ignore: trueis a clear config for “ignore all env vars”This minimal config cleanly expresses “acknowledge env access but ignore all env variables,” which is exactly what the new ignore semantics are targeting.
libs/config/deno_json/mod.rs (1)
43-44: Public re-exports of ignore-aware permission types are appropriateRe-exporting
AllowDenyIgnorePermissionConfigandAllowDenyIgnorePermissionConfigValuealongside the existing Allow/Deny types cleanly surfaces the new env-ignore configuration model to callers without disrupting existing APIs.runtime/ops/permissions.rs (1)
35-46: MappingPermissionState::Ignoredto"denied"is consistent with permission semanticsTreating
IgnoredalongsideDenied/DeniedPartialas"denied"keeps the JS-facingPermissionStatussimple and correctly reflects that access is not granted; keepingpartialtied only toGrantedPartialpreserves existing behavior.cli/schemas/config-file.v1.json (1)
19-35: Env permission schema cleanly supports allow/deny/ignore withoutallOfconflictsDefining
allowDenyIgnorePermissionConfigexplicitly and pointingpermissionSet.envatallowDenyIgnorePermissionConfigValuelets env permissions use boolean/array or an object withallow/deny/ignore, while keepingadditionalProperties: falsesafe and consistent. This matches the new ignore semantics without breaking existing config shapes.Also applies to: 72-72
cli/args/mod.rs (1)
1719-2017: Tests correctly validate env allow/deny/ignore resolution intoPermissionsOptionsThe updated test imports
AllowDenyIgnorePermissionConfig, setsenvwith allow/deny/ignore, and asserts thatflags_to_permissions_optionsproducesignore_env: Some(["env-ignore"])in the first scenario andignore_env: Nonewhenpermissions.all = Some(true)and no explicit ignore is configured. This provides solid coverage for the new env-ignore configuration path and its interaction with the globalallsetting.runtime/permissions/lib.rs (4)
127-139: Ignored permission state is wired consistently into query/check behaviorAdding
PermissionState::Ignoredand threading it throughUnaryPermission::query_desc,is_allow_all, andis_partial_flag_deniedmatches the intended “internal ignore” semantics:
is_allow_all()now correctly treats any ignored global or descriptor as disqualifying a permission from being “fully granted”, preventing audit‑skip short‑circuits when ignores are configured.- Exact matches on ignored descriptors (or a global ignore flag) yield
PermissionState::Ignored, while non‑matching descriptors still pass through the existing Granted/Denied/Partial logic.is_partial_flag_deniedtreatingFlagIgnoredalongsideFlagDeniedonly for the “no descriptor” case gives the desired DeniedPartial semantics forcheck_all()while keeping per‑descriptor queries precise.This looks coherent and backwards‑compatible for non‑env permissions.
Also applies to: 939-947, 989-1014, 1139-1154
891-899: UnaryPermission global ignore flags and child propagation look correctThe addition of
flag_ignored_globaltoUnaryPermissionand its usage in:
Default/Clone,is_allow_all()(via!self.flag_ignored_globaland!descriptors.has_any_denied_or_ignored()), andcreate_child_permissions()(copying both the global flag andFlagIgnoreddescriptors into the child),ensures:
- Global ignores prevent “fully granted” fast‑paths.
- Ignore state is faithfully inherited by worker/child permissions, avoiding accidental privilege escalation in workers.
This matches how global denies are handled and keeps ignore semantics consistent across parent/child permissions.
Also applies to: 901-912, 914-925, 1199-1248
3372-3392: ignore_env plumbing via new_unary_with_ignore is generic and appears correct
PermissionsOptionsgaining anignore_envfield and the introduction ofPermissions::new_unary_with_ignoregive a clean, generic way to construct permissions from allow/deny/ignore lists:
- Capacity preallocation accounts for all three lists, and
global_from_optionproperly interprets an explicit empty list as a “global” flag for each of allow/deny/ignore.new_unary()now just delegates tonew_unary_with_ignorewithignore_list = None, so existing callers stay unchanged.Permissions::from_optionswires env up vianew_unary_with_ignore, while other permissions still usenew_unary, keeping ignore semantics constrained to env as intended.Permissions::none(...)still usesnew_unaryfor env, so ignore behavior is only active when explicitly configured via flags/config.Overall this is a solid extension point; if you later decide to support ignore semantics for other permission types, this structure will generalize easily.
Also applies to: 3411-3451, 3567-3578, 3640-3650
1780-1781: NetDescriptor ordering and comparisons align with new precedence logicDeriving
PartialOrd/OrdforNetDescriptorand implementingAllowDescriptor::cmp_allow/cmp_denyandDenyDescriptor::cmp_denywith host/port‑aware ordering looks consistent:
cmp_allowtreats more specific descriptors (with ports) as “less than” their host‑only counterparts, which fits the “more specific first” rule used throughout the permission descriptor ordering.cmp_denylayering on top ofcmp_allowwithEqual => Greaterpreserves the intended “deny wins over allow” behavior when descriptors are identical.- The
NetDescriptortests (test_net_ordering_with_ports,test_cmp_net_descriptors, etc.) exercise both allow/deny interactions and the new ordering rules, which is reassuring given how central these comparisons are to permission resolution.I don’t see any ordering inconsistencies here.
Also applies to: 1857-1884, 1922-2052
nathanwhit
left a comment
nathanwhit
left a comment
There was a problem hiding this comment.
LGTM, just a couple small comments
| PermissionState::DeniedPartial | PermissionState::Denied => "denied", | ||
| PermissionState::Ignored | ||
| | PermissionState::DeniedPartial | ||
| | PermissionState::Denied => "denied", |
There was a problem hiding this comment.
Should we add a new "ignored" permission status or is denied fine?
There was a problem hiding this comment.
I didn't want to change the public runtime api, but probably.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
runtime/permissions/lib.rs (1)
723-812:UnaryPermissionDescordering withFlagIgnoredlooks sound but numeric precedence remains opaqueThe
AllowOrDenyDescRef::Deny(_, u8)pluskind_precedence/localprecedencecleanly extend the existing ordering to a third deny‑like kind, and the logic inOrd::cmpstill obeys the expected invariants (deny vs allow, more specific before less specific, tests at the bottom cover the non‑Ignored cases).One minor readability nit: the magic
0/1/2precedence values forFlagDenied,PromptDenied, andFlagIgnoredare still non‑self‑documenting. A small internal enum or namedconstvalues for these precedence levels would make this easier to maintain, especially now that there are three states instead of two.
🧹 Nitpick comments (2)
cli/args/mod.rs (1)
1549-1715: Deny/ignore helper and ignore_env wiring look consistent and correctThe new
handle_deny_or_ignorehelper correctly mirrors the existing precedence semantics: CLI vectors override config,PermissionConfigValue::Allis encoded asSome(vec![]), andNonekeeps the field unset. Reusing it for alldeny_*fields plusignore_envkeeps the behavior uniform and makes sentinel"all"semantics for--ignore-...and config"ignore": "all"align with existing deny handling.The interaction with
flags.allow_all(dropping config while still honoring explicit CLI deny/ignore flags) also seems intentional and reasonable for env, net, fs, etc. Overall this centralization should be low‑risk and improves maintainability.If you want to further clarify behavior later, a small comment on
handle_deny_or_ignorenoting thatSome(vec![])means “all denied/ignored” could help future readers, but not required for this PR.runtime/permissions/lib.rs (1)
6509-6565:test_env_ignorecovers key allow/deny/ignore precedence cases; consider adding a global‑ignore checkThe new test validates both:
- Simple allow vs deny vs ignore (
ALLOWED_*,DENIED_*,IGNORED_*).- Overlapping patterns where a more specific ignore (
PREFIX_IGNORED*) and allow (PREFIX_ALLOWED*) must outrank a broader deny (PREFIX*).This gives good coverage of the intended precedence rules. As a small enhancement, you might add a case for the “global ignore” form (
ignore_env: Some(vec![])) to assert that queries reportPermissionState::Ignoredboth for specific vars and forquery(None), mirroring the CLI--ignore-envbehavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cli/args/mod.rs(12 hunks)cli/schemas/config-file.v1.json(2 hunks)runtime/permissions/lib.rs(25 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cli/schemas/config-file.v1.json
🧰 Additional context used
🧬 Code graph analysis (1)
runtime/permissions/lib.rs (3)
cli/tsc/dts/lib.deno.ns.d.ts (1)
PermissionState(4062-4062)runtime/permissions/prompter.rs (4)
prompt(96-103)prompt(110-119)prompt(321-525)prompt(538-551)runtime/ops/permissions.rs (1)
from(36-47)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: test debug linux-x86_64
- GitHub Check: test debug linux-aarch64
- GitHub Check: test release windows-x86_64
- GitHub Check: test debug windows-x86_64
- GitHub Check: test debug macos-x86_64
- GitHub Check: test release macos-aarch64
- GitHub Check: test debug macos-aarch64
- GitHub Check: test release macos-x86_64
- GitHub Check: lint debug windows-x86_64
- GitHub Check: lint debug linux-x86_64
- GitHub Check: lint debug macos-x86_64
- GitHub Check: build libs
🔇 Additional comments (8)
cli/args/mod.rs (1)
1719-2015: Tests cover env ignore config and new PermissionsOptions fieldThe test updates (using
AllowDenyIgnorePermissionConfigforenvand assertingignore_envinPermissionsOptions) solidly exercise the new wiring from config →flags_to_permissions_options. The first case validates non‑emptyenv.ignorepropagation; the second checks thatall: Some(true)does not implicitly enable ignore and that the defaultignore_envisNone.This gives good regression coverage around the new field without overcomplicating the fixture.
runtime/permissions/lib.rs (7)
129-141: NewPermissionState::Ignoredis consistent with existing status mappingIntroducing
Ignoredas a distinct internal state while still mapping it to"denied"for the publicPermissionStatus(viaruntime/ops/permissions.rs) keeps the JS surface stable and gives env ops a clean hook to “soft‑deny” reads. The enum layout and discriminant choice look fine; I don’t see any ordering or matching pitfalls here.
815-921: Trackinghas_flag_ignored/flag_ignored_globalintegrates cleanly intois_allow_allAdding
has_flag_ignoredtoUnaryPermissionDescriptorsandflag_ignored_globaltoUnaryPermission, and folding them intohas_any_denied_or_ignored()andis_allow_all(), correctly ensures that any explicit ignore (global or per‑descriptor) prevents a permission from being treated as “fully granted”. That matches the semantics that “ignore” is still a restriction and should block fast‑path/all‑granted optimizations.
991-1050: Env ignore semantics inquery_descand child permissions are logically consistent
query_allowed_desc_for_exact_matchnow returnsPermissionState::Ignoredwhen a query matches aFlagIgnoreddescriptor, andquery_descshort‑circuits toIgnoredwhenflag_ignored_globalis set. This gives a clear, first‑class “ignored” result that callers (env ops, Deno.permissions.*) can distinguish fromDeniedandPrompt.is_partial_flag_deniedtreatingFlagIgnoredthe same asFlagDeniedfor partial/“*_Partial” states is reasonable: ignores are still effectively denials when deciding if a permission is partially constrained.create_child_permissionspropagatesflag_ignored_globaland carries over allFlagIgnoreddescriptors, preserving ignore semantics for workers and preventing privilege escalation (since the child allow‑list is still checked against the parent’s permissions before cloning).Given the new unit tests and env‑wildcard tests, this path looks correct.
Also applies to: 1141-1156, 1201-1253
1782-2057:NetDescriptorrework (Query/Allow/Deny + ordering) matches intended host/port semanticsMaking
NetDescriptoritself theQueryDesc,AllowDesc, andDenyDescsimplifies the model and the new comparison logic behaves as expected:
cmp_allowprefers more specific descriptors (with port) before less specific ones (no port), and then orders byHostthen port, which aligns with the new tests intest_net_ordering_with_portsandtest_cmp_net_descriptors.AllowDescriptor::cmp_denybiases towards deny entries when descriptors are equal, whileDenyDescriptor::cmp_denyuses the same structural ordering as allows, which fits the general pattern used for other permission types and keepsUnaryPermissionDesc’s sort stable.- Parsing in
parse_innerstill covers vsock, bracketed IPv6, IPv4/FQDN with optional ports, and rejects URL‑like inputs, with extensive tests for bothparse_for_queryandparse_for_list.I don’t see any net‑permission regressions in this implementation.
3374-3394:PermissionsOptions.ignore_envis a minimal, backwards‑compatible extensionAdding
ignore_env: Option<Vec<String>>toPermissionsOptionswith existing serde derives provides the new configuration surface without impacting other permissions. No issues here; it’s structurally consistent withallow_env/deny_env.
3413-3452:Permissions::new_unary_with_ignoreand wrappernew_unaryclean up constructionThe new helper cleanly centralizes allow/deny/ignore list handling:
- Capacity preallocation accounts for all three lists.
global_from_optionfor each list keeps the existing “empty vector means global flag” convention and extends it toignore.- The struct update
..Default::default()only fillsprompt_denied_global(and future fields), avoiding duplication.Using
new_unaryas a thin wrapper for non‑ignore permissions keeps call sites simple while allowing env to opt into ignore semantics. This looks well‑factored.
3569-3580: Env permissions correctly wired toignore_envinPermissions::from_optionsThe env construction now passes three independently parsed lists into
new_unary_with_ignore, so CLI/config combinations like:
--allow-env=...--deny-env=...--ignore-env=.../--ignore-env(empty list for global ignore)all map directly into the internal representation. Parsing paths are symmetric with the existing allow/deny env handling, and other permissions still go through
new_unary, so this change is nicely localized.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
runtime/permissions/lib.rs (3)
129-141:PermissionState::Ignoredis wired consistently but consider documenting intentThe new
Ignoredvariant integrates cleanly with the rest of this module and, viaPermissionStatusmapping, will surface as"denied"to JS callers, which matches the “return undefined instead of throwing” semantics for env. One small follow-up: the/// Quadri-statecomment above the enum is now inaccurate and doesn’t explain howIgnoredis intended to differ fromDeniedfrom the engine’s perspective; a brief doc tweak here would help future readers.
822-898: Ignored tracking inUnaryPermissionand query logic correctly mirror deny semanticsThe additions of:
has_flag_ignoredandhas_any_denied_or_ignoredinUnaryPermissionDescriptors,flag_ignored_globalinUnaryPermission,- the new branches in
is_allow_all,query_desc,query_allowed_desc_for_exact_match,is_partial_flag_denied, and- propagation of
flag_ignored_globalandFlagIgnoreddescriptors increate_child_permissionsare internally consistent and match the intended behavior:
is_allow_allnow returnsfalsewhenever any explicit deny, prompt-deny, or ignore exists (global or descriptor-based), preventing the audit macro from bypassing ignore logic.query_descreturnsIgnoredeither when an exactFlagIgnoreddescriptor matches or whenflag_ignored_globalis set, giving a clear signal for env ops to treat reads as “acknowledged but value hidden.”is_partial_flag_deniednow treats ignored descriptors like denied ones for the global (None) case, soDeno.permissions.query({ name: "env" })can correctly report partial states when there are ignored patterns, while per-var env queries still rely on the specific match ordering and are unaffected becauseEnvQueryDescriptor::overlaps_denyisfalse.create_child_permissionscopies both global and per-descriptor ignores from parent to child, and escalation checks run through the samecheck_in_permissionpath, so children can’t “un-ignore” a parent’s env patterns.Given the targeted tests (
test_env_wildcards,test_env_ignore) and existing path/sys/net/ffi/import tests, this design looks correct and robust.Also applies to: 900-955, 998-1023, 1048-1163, 1208-1258
6516-6572:test_env_ignoregives good coverage of ignore vs allow vs deny precedenceThe new test exercises both disjoint and overlapping prefix patterns for env:
- First block validates that
ALLOWED_*,IGNORED_*, andDENIED_*lead toGranted,Ignored, andDeniedrespectively.- Second block checks nested patterns where a broad deny (
PREFIX*) coexists with a more specific allow (PREFIX_ALLOWED*) and a more specific ignore (PREFIX_IGNORED*), and the outcomes (Denied,Ignored,Granted) are as desired.This nicely anchors the intended ordering behavior for
FlagIgnoredwithout impacting existing env wildcard tests. If you later support global-only ignore without any allow/deny, consider adding a small case forignore_env: Some(vec![])to lock in the “everything ignored” behavior as well, but it’s not strictly necessary for this PR.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
runtime/permissions/lib.rs(25 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
runtime/permissions/lib.rs (3)
cli/tsc/dts/lib.deno.ns.d.ts (1)
PermissionState(4062-4062)runtime/permissions/prompter.rs (4)
prompt(96-103)prompt(110-119)prompt(321-525)prompt(538-551)runtime/ops/permissions.rs (1)
from(36-47)
🔇 Additional comments (3)
runtime/permissions/lib.rs (3)
723-820: Ordering changes forUnaryPermissionDesc(includingFlagIgnored) look soundMoving
AllowOrDenyDescRef::Denyto a struct with an explicitorderand extendingUnaryPermissionDescwithFlagIgnoredplus the updatedOrdimplementation provides a clear, deterministic total ordering:
- For equal descriptors, precedence is
FlagDenied(0) <PromptDenied(1) <FlagIgnored(2) <Granted(3).- For different descriptors, the existing allow/deny
cmp_*semantics remain intact, so “more specific” descriptors are still favored regardless of type.Given the extensive comparison tests at the bottom of the file for read/write/net/env/sys/ffi/import descriptors (which still exercise the same ordering logic), this looks correct and backward-compatible, with
FlagIgnorednaturally treated as the weakest deny flavor.
1789-1791: DerivingPartialOrd/OrdonNetDescriptoris reasonable but keep it aligned withcmp_allowAdding
PartialOrd/OrdforNetDescriptormatches its existingHost/Option<u32>ordering and should be safe, as permission ordering still goes throughcmp_allow/cmp_deny. Just be mindful that any new code that relies onNetDescriptor’s derivedOrdshould have expectations consistent withcmp_allow’s notion of specificity (ports vs host-only), to avoid subtle mismatches later.
3381-3401:ignore_envplumbing andnew_unary_with_ignoreconstruction match the desired semanticsThe new pieces work together as expected:
PermissionsOptions.ignore_env: Option<Vec<String>>is parsed alongsideallow_env/deny_env.Permissions::new_unary_with_ignoreconstructs aUnaryPermissionwith:
granted_global/flag_denied_global/flag_ignored_globaldriven by empty-vector-as-global viaglobal_from_option,- descriptors containing any explicit allow/deny/ignore entries, and
- correct capacity preallocation.
Permissions::new_unaryis now just a thin wrapper for the old two-list behavior (ignore_list=None), so existing callers remain behaviorally identical.- In
from_options, onlyenvusesnew_unary_with_ignore, feeding in parsed allow/deny/ignore env descriptors, so other permissions are untouched.This gives the intended combinations:
--ignore-envalone ⇒flag_ignored_global = true, no allows/denies ⇒ per-var queries returnIgnored, and enumeration can be handled non-throwingly inext/os.--ignore-env=VAR1 --allow-env=VAR2⇒VAR1yieldsIgnored,VAR2Granted, others follow the existing allow/deny rules.--allow-env --deny-env=SOMETHINGsemantics are unchanged at the engine level, so the new “no error on enumeration” behavior can be implemented at the env-op layer by interpretingDenied/Ignoredas “filter out.”This wiring looks correct and matches the PR objectives.
Also applies to: 3419-3459, 3550-3587
3 participants
Adds the ability to ignore certain environment variables and return
undefinedinstead of erroring.Also doesn't error anymore when doing
--allow-env --deny-env=SOMETHINGand getting all env vars.Closes #30891
Closes #31071